core/aop-runtime/test/fixtures/modules/hello_cross_cut/CallTrace.ts (3)

3-10: LGTM: Well-structured interface with a minor improvement suggestion.

The CallTraceMsg interface provides a clear contract for call trace messages. However, consider using a more specific type for adviceParams instead of any to improve type safety.

You could replace any with a more specific type or use a generic type parameter:

export interface CallTraceMsg<T = unknown> {
  // ... other properties
  adviceParams?: T;
}

This allows users of the interface to specify the type of adviceParams when needed, while still maintaining flexibility.

---

12-21: LGTM: Well-implemented singleton class with potential for enhancement.

The CallTrace class is correctly implemented as a singleton with public access, which is suitable for a global call trace collector. The addMsg method serves its purpose well.

Consider adding the following methods to enhance functionality:

1. A method to retrieve messages:

getMessages(): Array<CallTraceMsg> {
  return [...this.msgs]; // Return a copy to prevent external modifications
}

2. A method to clear messages:

clearMessages(): void {
  this.msgs = [];
}

These additions would provide more control over the collected messages and could be useful for testing or debugging purposes.

---

1-21: Enhance file documentation for clarity and usage guidance.

While the implementation is solid, the file could benefit from additional documentation to explain its purpose and usage, especially considering its location in the test fixtures directory.

Consider adding a file-level JSDoc comment at the beginning of the file:

/**
 * This file defines structures and a singleton class for collecting call trace messages.
 * It is primarily used for testing and debugging Aspect-Oriented Programming (AOP) functionality.
 * 
 * @module CallTrace
 */

This documentation would help other developers understand the purpose and context of this code more quickly.

core/test-util/CoreTestHelper.ts (2)

8-14: LGTM with a minor suggestion.

The new imports are correctly added and necessary for the changes in the prepareModules method. However, to maintain consistency with the project's coding style and address the static analysis hint, please add a trailing comma after the last import.

Apply this small change:

 import {
   EggLoadUnitType,
   EggPrototype,
   GlobalGraph,
   GlobalGraphBuildHook,
-  LoadUnitFactory
+  LoadUnitFactory,
 } from '@eggjs/tegg-metadata';

🧰 Tools

🪛 GitHub Check: Runner-ubuntu (22)

[failure] 13-13:
Missing trailing comma

🪛 GitHub Check: Runner-ubuntu (20)

[failure] 13-13:
Missing trailing comma

🪛 GitHub Check: Runner-ubuntu (18)

[failure] 13-13:
Missing trailing comma

🪛 GitHub Check: Runner-macos (20)

[failure] 13-13:
Missing trailing comma

🪛 GitHub Check: Runner-macos (18)

[failure] 13-13:
Missing trailing comma

🪛 GitHub Check: Runner-macos (16)

[failure] 13-13:
Missing trailing comma

🪛 GitHub Check: Runner-ubuntu (16)

[failure] 13-13:
Missing trailing comma

---

41-47: LGTM with suggestions for improved robustness and clarity.

The changes to the prepareModules method look good overall. The addition of the hooks parameter provides more flexibility in the graph building process. However, I have two suggestions to improve the code:

1. Add a null check for GlobalGraph.instance to prevent potential runtime errors:

-    for (const { path } of GlobalGraph.instance!.moduleConfigList) {
+    if (!GlobalGraph.instance) {
+      throw new Error('GlobalGraph instance is not initialized');
+    }
+    for (const { path } of GlobalGraph.instance.moduleConfigList) {

2. Add a comment explaining the relationship between moduleDirs and GlobalGraph.instance!.moduleConfigList to improve code clarity:

// Note: GlobalGraph.instance!.moduleConfigList is populated by LoaderUtil.buildGlobalGraph
// and should contain the same paths as moduleDirs
for (const { path } of GlobalGraph.instance!.moduleConfigList) {
  // ...
}

These changes will make the code more robust and easier to understand for future maintainers.

core/aop-decorator/src/AspectMetaBuilder.ts (2)

Line range hint 29-46: Consider optimizing and improving type safety of getAllMethods

While the current implementation of getAllMethods is functional, consider the following improvements:

1. Add type annotations for better type safety:

static getAllMethods(clazz: EggProtoImplClass): PropertyKey[] {
  const methodSet = new Set<PropertyKey>();
  // ... rest of the implementation
}

2. Move the inner getMethods function outside to avoid recreation on each call:

private static getMethods(obj: any, methodSet: Set<PropertyKey>): void {
  if (obj) {
    const propDescs = Object.getOwnPropertyDescriptors(obj);
    for (const [name, desc] of Object.entries(propDescs)) {
      if (desc.value instanceof Function) {
        methodSet.add(name);
      }
    }
    this.getMethods(Object.getPrototypeOf(obj), methodSet);
  }
}

static getAllMethods(clazz: EggProtoImplClass): PropertyKey[] {
  const methodSet = new Set<PropertyKey>();
  this.getMethods(clazz.prototype, methodSet);
  return Array.from(methodSet);
}

3. Consider using a loop instead of recursion for better performance with deep prototype chains:

static getAllMethods(clazz: EggProtoImplClass): PropertyKey[] {
  const methodSet = new Set<PropertyKey>();
  let obj = clazz.prototype;
  while (obj) {
    const propDescs = Object.getOwnPropertyDescriptors(obj);
    for (const [name, desc] of Object.entries(propDescs)) {
      if (desc.value instanceof Function) {
        methodSet.add(name);
      }
    }
    obj = Object.getPrototypeOf(obj);
  }
  return Array.from(methodSet);
}

These changes could improve type safety, reduce memory usage, and potentially improve performance for classes with deep prototype chains.

---

Line range hint 1-58: Enhance documentation and consider refactoring AspectMetaBuilder

The AspectMetaBuilder class is well-structured, but consider the following improvements:

1. Add comprehensive class-level documentation explaining the purpose and usage of AspectMetaBuilder.

2. Consider refactoring doBuildMethodAspect for better readability:

private doBuildMethodAspect(method: PropertyKey): Aspect | undefined {
  const crosscutAdviceList = this.getCrosscutAdvice(method);
  const pointcutAdviceList = this.getPointcutAdvice(method);
  
  if (!this.hasAdvice(crosscutAdviceList, pointcutAdviceList)) return;
  
  return this.buildAspect(method, crosscutAdviceList, pointcutAdviceList);
}

private getCrosscutAdvice(method: PropertyKey) {
  return this.crosscutAdviceFactory.getAdvice(this.clazz, method);
}

private getPointcutAdvice(method: PropertyKey) {
  return PointcutAdviceInfoUtil.getPointcutAdviceInfoList(this.clazz, method);
}

private hasAdvice(crosscutAdviceList: any[], pointcutAdviceList: any[]): boolean {
  return crosscutAdviceList.length > 0 || pointcutAdviceList.length > 0;
}

private buildAspect(method: PropertyKey, crosscutAdviceList: any[], pointcutAdviceList: any[]): Aspect {
  const aspectBuilder = new AspectBuilder(this.clazz, method);
  [...crosscutAdviceList, ...pointcutAdviceList].forEach(advice => aspectBuilder.addAdvice(advice));
  return aspectBuilder.build();
}

3. Improve error handling, especially for edge cases. For example, add checks for null or undefined values in the constructor and methods.

4. Consider adding logging for important operations or potential issues.

These changes would improve the maintainability and robustness of the AspectMetaBuilder class.

core/test-util/LoaderUtil.ts (3)

37-37: LGTM: Method signature updated correctly with a minor suggestion.

The buildGlobalGraph method signature has been appropriately updated to include the optional hooks parameter. This change enhances the flexibility of the graph building process.

Consider using a default value for the hooks parameter to simplify the method body:

static buildGlobalGraph(modulePaths: string[], hooks: GlobalGraphBuildHook[] = [])

This would eliminate the need for the null coalescing operator in the for loop.

---

39-41: LGTM: Hook registration loop implemented correctly with a minor suggestion.

The new loop correctly iterates over the hooks array and registers each hook with the GlobalGraph instance. This implementation aligns with the intended functionality of enabling the registration of build hooks during the graph construction process.

To improve readability and reduce nesting, consider using the Array.prototype.forEach method:

(hooks ?? []).forEach(hook => GlobalGraph.instance.registerBuildHook(hook));

This change would make the code more concise and easier to read.

---

Test coverage for buildGlobalGraph and GlobalGraphBuildHook is missing.

The recent changes to the LoaderUtil class introduce new functionalities related to buildGlobalGraph and GlobalGraphBuildHook. However, our verification indicates that there are no existing test cases covering these methods.

• Missing Tests:
  • buildGlobalGraph method with hooks
  • GlobalGraphBuildHook functionality

Please add unit tests to ensure these new features are properly tested and to maintain code reliability.

🔗 Analysis chain

Line range hint 2-41: Overall implementation looks good with a suggestion for test coverage.

The changes to the LoaderUtil class, including the updated import statement, method signature, and new hook registration loop, are well-implemented and consistent with the intended functionality. The modifications enhance the flexibility of the graph building process by allowing the registration of build hooks.

To ensure the new functionality is properly tested, please run the following script to check for test coverage of the buildGlobalGraph method:

If the script doesn't return any results, consider adding test cases to cover the new functionality with hooks.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify test coverage for the buildGlobalGraph method

# Test: Search for test cases that use the buildGlobalGraph method with hooks
rg --type typescript -g '*.test.ts' -g '*.spec.ts' 'buildGlobalGraph.*hooks'

# Test: Search for test cases that specifically test the hook functionality
rg --type typescript -g '*.test.ts' -g '*.spec.ts' 'GlobalGraphBuildHook'

Length of output: 222

---

Script:

#!/bin/bash
# Description: Verify test coverage for the buildGlobalGraph method

# Test: Search for test cases that use the buildGlobalGraph method with hooks
rg -g '*.test.ts' -g '*.spec.ts' 'buildGlobalGraph.*hooks'

# Test: Search for test cases that specifically test the hook functionality
rg -g '*.test.ts' -g '*.spec.ts' 'GlobalGraphBuildHook'

Length of output: 116

---

Script:

#!/bin/bash
# Description: Broadly verify test coverage for the buildGlobalGraph method and GlobalGraphBuildHook functionality

# Test: Search all TypeScript test files for usage of buildGlobalGraph method with hooks
rg -g '*.test.ts' -g '*.spec.ts' 'buildGlobalGraph\s*\(.*hooks'

# Test: Search all TypeScript test files for usage of GlobalGraphBuildHook
rg -g '*.test.ts' -g '*.spec.ts' 'GlobalGraphBuildHook'

# Test: Search any TypeScript files for buildGlobalGraph method implementations
rg -g '*.ts' 'buildGlobalGraph'

# Test: Search any TypeScript files for GlobalGraphBuildHook implementations
rg -g '*.ts' 'GlobalGraphBuildHook'

Length of output: 2480

core/aop-runtime/src/PointCutGraphHook.ts (1)

39-39: Consider handling missing property more gracefully

While the assert statement in line 39 ensures that property exists, relying on assertions might not be ideal in a production environment. Consider handling the case where property is missing with a proper error message or exception to provide clearer feedback.

For example:

  const property = PrototypeUtil.getProperty(clazz);
- assert(property, 'not found property');
+ if (!property) {
+   throw new Error(`Property not found for class ${clazz.name}`);
+ }

This provides a more informative error message and aligns with standard error-handling practices.

core/aop-runtime/test/fixtures/modules/hello_cross_cut/HelloCrossCut.ts (2)

37-37: Narrow the type of 'result' parameter

Using any for the result parameter reduces type safety. If possible, specify a more precise type based on what the hello method returns.

For example:

-async afterReturn(ctx: AdviceContext<Hello, AfterReturnParams>, result: any): Promise<void> {
+async afterReturn(ctx: AdviceContext<Hello, AfterReturnParams>, result: string): Promise<void> {

---

65-65: Avoid mutating 'ctx.args' directly

Directly modifying ctx.args can lead to unexpected side effects, especially if other advices rely on the original arguments. Consider creating a shallow copy of the arguments before modifying them.

Refactor the code as follows:

-ctx.args[0] = `withCrosscutAroundParam(${ctx.args[0]})`;
+const newArgs = [...ctx.args];
+newArgs[0] = `withCrosscutAroundParam(${ctx.args[0]})`;
+ctx.args = newArgs;

core/aop-runtime/src/CrossCutGraphHook.ts (1)

10-26: Consider Performance Implications of Nested Loops

In the crossCutGraphHook function, there are nested loops over the moduleGraph and protos, which might lead to performance issues if the graphs are large. Consider optimizing the iteration or adding conditions to reduce unnecessary processing.

One possible optimization is to filter the modules or protos before iterating:

 for (const moduleNode of globalGraph.moduleGraph.nodes.values()) {
+  if (!moduleNode.val.hasCrossCutProtos) continue;
   for (const crossCutProtoNode of moduleNode.val.protos) {

Ensure that any added properties like hasCrossCutProtos are properly maintained.

core/aop-runtime/test/fixtures/modules/hello_point_cut/HellloPointCut.ts (1)

13-13: Consider translating the comment to English for consistency

The comment on line 13 is in Chinese. For consistency and maintainability across the codebase, it's preferable to use English for code comments.

core/metadata/src/model/graph/GlobalGraph.ts (1)

75-76: Add documentation for registerBuildHook method

The registerBuildHook method allows registration of build hooks. Adding JSDoc comments or documentation would improve maintainability and help other developers understand its usage.

Configuration used: CodeRabbit UI
Review profile: CHILL